libct/cgroup: rm GetClosestMountpointAncestor using moby/sys/mountinfo parser #2401
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This function is not very efficient, does not really belong to cgroup package, and is only used once (from fs/cpuset.go). Prepare to remove it by replacing with the implementation based on the parser from github.com/moby/sys/mountinfo parser. This commit is here to make sure the proposed replacement passes the unit test. Funny, but the unit test need to be slightly modified since it supplies the wrong mountinfo (space as the first character, empty line at the end). Validated by $ go test -v -run Ance === RUN TestGetClosestMountpointAncestor --- PASS: TestGetClosestMountpointAncestor (0.00s) PASS ok github.com/opencontainers/runc/libcontainer/cgroups 0.002s Signed-off-by: Kir Kolyshkin <[email protected]>
The function GetClosestMountpointAncestor is not very efficient, does not really belong to cgroup package, and is only used once (from fs/cpuset.go). Remove it, replacing with the implementation based on moby/sys/mountinfo parser. Signed-off-by: Kir Kolyshkin <[email protected]>
|
OK, CI passed, adding the second commit |
|
I quickly checked Docker and libpod sources -- this function is not used in there. |
|
@AkihiroSuda @mrunalp PTAL |
|
LGTM, but I guess we may eventually move back the function to libct/cg . |
There is no need to mess with mountinfo for cgroup v2 case, and I don't think we'll make any major changes to cgroup v1 (except I'm still tinkering with how to decrease the ridiculous number of times we parse /proc/self/mountinfo). |
|
LGTM |
AkihiroSuda
added a commit
to AkihiroSuda/runc
that referenced
this pull request
May 19, 2020
The compilation error had ocurred because of a bad rebase during opencontainers#2401 and opencontainers#2413 Signed-off-by: Akihiro Suda <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The function
GetClosestMountpointAncestoris not very efficient,does not really belong to cgroup package, and is only used once
(from fs/cpuset.go).
The removal is done in two stages/commits:
1
Prepare to remove it by replacing with the implementation based on
the
github.com/moby/sys/mountinfoparser.This commit is here to make sure the proposed replacement passes the
unit test.
Funny, but the unit test need to be slightly modified since it
supplies the wrong mountinfo (space as the first character, empty line
at the end).
Validated by
2
Actually remove
GetClosestMountpointAncestor, move the replacementcode to
libcontainer/cgroups/fs/cpuset.go.